Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timers: fix refresh inside callback #26721

Closed

Conversation

apapirovski
Copy link
Member

When timers.refresh() is called inside a callback, the timer would incorrectly end up unrefed and thus not keep the event loop alive.

Fixes: #26642

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 17, 2019
@apapirovski
Copy link
Member Author

@antsmartian antsmartian added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2019
When `timers.refresh()` is called inside a callback, the timer would
incorrectly end up unrefed and thus not keep the event loop alive.
@apapirovski apapirovski force-pushed the fix-timers-refresh-in-callback branch from 239c9ae to a70719a Compare March 19, 2019 05:42
@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/21655/ (had to rebase significantly)

@antsmartian
Copy link
Contributor

@danbev
Copy link
Contributor

danbev commented Mar 20, 2019

Landed in 4306300.

@danbev danbev closed this Mar 20, 2019
danbev pushed a commit that referenced this pull request Mar 20, 2019
When `timers.refresh()` is called inside a callback, the timer would
incorrectly end up unrefed and thus not keep the event loop alive.

PR-URL: #26721
Fixes: #26642
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
When `timers.refresh()` is called inside a callback, the timer would
incorrectly end up unrefed and thus not keep the event loop alive.

PR-URL: #26721
Fixes: #26642
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
zero1five added a commit to zero1five/node that referenced this pull request Jun 13, 2019
Add an or option for put refresh back to work. nodejs#26721 one reason it
can't be overridden it only works in the callback of the current
timer(before `finally`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v11.11.0 timeout.refresh() does not seem to prevent event loop from exiting
9 participants